RHF last domino: instance create + network interfaces #1274
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // API accepts undefined but it's easier if we don't | ||
| SetRequired<InstanceCreate, 'networkInterfaces'>, | ||
| { | ||
| networkInterfaceType: InstanceNetworkInterfaceAttachment['type'] |
There was a problem hiding this comment.
What I've done here is take the None/Default/Custom radio state out of the top-level form state and used networkInterfaces.type as the value of the field.
There was a problem hiding this comment.
Yeah, definitely like that better.
| > | ||
|
|
||
| const values: InstanceCreateInput = { | ||
| const baseDefaultValues: InstanceCreateInput = { |
There was a problem hiding this comment.
"base" because we add in the default value for the selected image at render time because it comes from the API.
| }: CreateFormProps<InstanceCreateInput, Instance>) { | ||
| CreateInstanceForm.loader = async () => { | ||
| await apiQueryClient.prefetchQuery('systemImageList', {}) | ||
| } |
There was a problem hiding this comment.
This means we always have the images on hand in render.
There was a problem hiding this comment.
I assume that's just the default pagination size? We really have to rework the images API... or figure out how images are even supposed to exist in the product.
There was a problem hiding this comment.
Yeah, default of 1000
| * then in the calling code, the field would not infer `TFieldValues` and | ||
| * constrain the `name` prop to paths in the values object. | ||
| */ | ||
| children: (form: UseFormReturn<TFieldValues>) => ReactNode |
There was a problem hiding this comment.
I realized that passing the whole form instead of only the control is obviously the thing to do here. In most calling code, all we change is (control) => to ({ control }) =>, so that's easy. I didn't change the SideModalForm to work this way yet because of noise. I'll do that in a followup when I move everything back into app/components/form.
Initially I made this change because I thought I needed it for instance create — I was going to do something funky with watch — but even though I didn't end up using it, it seems to me clearly the correct way to do it.
| </NavLinkItem> | ||
| <NavLinkItem to={pb.hotkeys()}> | ||
| <Profile16Icon /> Hotkeys | ||
| </NavLinkItem> |
There was a problem hiding this comment.
I took these out because they're fake and it seemed easier than changing them, but I can put them back.
There was a problem hiding this comment.
It's fine. Can just re-add them as we implement them.
| { | ||
| id: 'a2ea1d7a-cc5a-4fda-a400-e2d2b18f53c5', | ||
| name: 'ubuntu-20.04', | ||
| name: 'ubuntu-20-04', |
There was a problem hiding this comment.
These were invalid names and they were breaking the instance create POST! This is a bug on main actually.
just-be-dev
left a comment
just-be-dev
left a comment
There was a problem hiding this comment.
This was a big change, good work. Only minor feedback, excited to see formik removed.
| ...props, | ||
| })) | ||
| ) | ||
| export const Form = { |
There was a problem hiding this comment.
This nested declaration likely makes the babel plugin not work for these components.
| const [currentDistro, setCurrentDistro] = useState<string | undefined>(id) | ||
| control: Control<InstanceCreateInput> | ||
| }) { | ||
| const distros = images.map((image) => ({ ...image, ...distroDisplay(image) })) |
There was a problem hiding this comment.
Ah, nice, yeah. Cleaner.
| @@ -1,39 +0,0 @@ | |||
| import { Settings24Icon } from '@oxide/ui' | |||
There was a problem hiding this comment.
We'll need to re-add this page soon, but nbd.
NetworkInterfaceFieldImageSelectFieldRadioFieldtypingRadioField2, think about reworking it to be less repetitiveMain app.js size goes down 100 KB.